feat: clickhouse warehouse setup UI#8028
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ClickHouse warehouse request and response types, form validation, payload construction, and tests. Introduces feature-flagged ClickHouse selection, creation, editing, and connection testing in warehouse settings. Updates warehouse cards to display status details, host information, and conditional test actions. Moves shared save-button label logic into a utility and restricts polling to Flagsmith connections. Estimated code review effort: 3 (Moderate) | ~25 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/clickhouse-byo-warehouse-backend #8028 +/- ##
=========================================================================
+ Coverage 98.65% 98.67% +0.02%
=========================================================================
Files 1507 1514 +7
Lines 59866 60930 +1064
=========================================================================
+ Hits 59060 60124 +1064
Misses 806 806 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Docker builds report
|
|
@themis-blindfold review |
⚖️ Themis judgement: ✅ Ship itAdds the ClickHouse warehouse connection type to the frontend, gated behind the The form validation logic and payload builder are extracted into a standalone module with thorough unit tests covering port range validation, required-field checks, edit-mode password handling, and credential separation. The polling guard correctly limits automatic test-connection calls to Flagsmith-type connections while ClickHouse uses manual "Test connection" instead.
📝 Walkthrough
🧪 How to verify
Automate: the unit tests already cover the form logic; a Cypress/E2E test for the full create-edit-test-delete flow would close the gap. Product take: Solid feature addition that puts ClickHouse on equal footing with the Flagsmith warehouse type for BYO-warehouse users. Properly gated behind a flag so it ships dark — low-risk incremental rollout. 🧭 Assumptions & unverified claims
A warehouse that connects on the first try — now that's a feature worth flagging. · reviewed at af485e8 |
af485e8 to
19b31a1
Compare
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
|
Visual Regression19 screenshots compared. See report for details. |
|
@themis-blindfold review once again but this time focus on the frontend design patterns to make sure the used components follow the snowflake designs. Let's flag whether some scss or classes are duplicated, or could be better implemented. |
- Only render Test connection button when the handler is provided (backend only supports ClickHouse, not Snowflake) - Add missing id attributes on ClickHouse form buttons for E2E - Extract shared getButtonLabel helper from both config forms - Extract WarehouseConfigValue named type in requests.ts - Extract WarehouseConfigResponse named type in responses.ts
aa07e2f to
42149a9
Compare
|
@themis-blindfold review |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
| warningMessage={`We couldn't establish a connection${ | ||
| testDetail ? `: ${testDetail}` : '.' |
There was a problem hiding this comment.
🧹 Nit · ⚡ Quick win
Warning message missing period between testDetail and continuation sentence
Observed: when testDetail is e.g. "Authentication failed", the rendered string is "We couldn't establish a connection: Authentication failed You can save anyway..." — no sentence break before "You".
The falsy branch correctly appends '.', but the truthy branch leaves it to the backend value, which may or may not end with punctuation.
| warningMessage={`We couldn't establish a connection${ | |
| testDetail ? `: ${testDetail}` : '.' | |
| testDetail ? `: ${testDetail}.` : '.' |
⚖️ Themis judgement: 🧹 Ship it, nits insideWell-scoped feature addition: a ClickHouse warehouse config form behind a feature flag, mirroring the existing Snowflake form with the addition of a test-before-save flow. The credential/config separation is clean, validation logic is extracted and unit-tested, and the feature flag gating is correct. The new
🧹 Nits
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Automate: Product take: Adds ClickHouse as a second warehouse destination, meaningfully expanding the warehouse integration surface. Solid improvement for users running ClickHouse — the test-before-save flow is a better UX than the Snowflake form currently offers. 🧭 Assumptions & unverified claims
A warehouse that tests its own connection before saving — if only all relationships worked that way. · reviewed at 42149a9 |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/ClickHouseConfigForm.tsx (1)
1-19: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStill not moved into its own component folder; still imports Snowflake's stylesheet.
This new component remains directly under
warehouse-tab/and still imports./ConfigForm.scss(Snowflake's stylesheet) instead of a co-located one. This was already flagged on an earlier commit and remains unresolved.As per path instructions, "Each new component must live in its own folder with a barrel
index.ts, aComponentName/ComponentName.tsxfile, co-locatedComponentName.scss, any subcomponents, and imports should use the component folder path rather than the inner file."♻️ Proposed structure
ClickHouseConfigForm/ index.ts // export { default } from './ClickHouseConfigForm' ClickHouseConfigForm.tsx ClickHouseConfigForm.scss // own styles, or explicitly shared partialSource: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e7a6ddfa-32ca-4f81-bc2b-a2fd55b70dd1
📒 Files selected for processing (11)
frontend/common/services/useWarehouseConnection.tsfrontend/common/types/requests.tsfrontend/common/types/responses.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/ClickHouseConfigForm.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/ConfigForm.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseConnectionCard.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetup.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/__tests__/clickhouseConfig.test.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseConfig.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/index.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/warehouseFormUtils.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/ClickHouseConfigForm.tsx (1)
19-19: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winComponent still isn't in its own folder; still imports Snowflake's stylesheet directly.
This mirrors a previous review finding that hasn't been addressed in this revision: the component lives flat under
warehouse-tab/and pulls in./ConfigForm.scssrather than having a co-located stylesheet.As per path instructions, "Each new component must live in its own folder with a barrel
index.ts, aComponentName/ComponentName.tsxfile, co-locatedComponentName.scss, any subcomponents, and imports should use the component folder path rather than the inner file."♻️ Proposed structure
ClickHouseConfigForm/ index.ts // export { default } from './ClickHouseConfigForm' ClickHouseConfigForm.tsx ClickHouseConfigForm.scss // own styles, or explicitly re-export the shared partialSource: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f1f631be-943c-4c15-acd5-dea8089fd693
📒 Files selected for processing (11)
frontend/common/services/useWarehouseConnection.tsfrontend/common/types/requests.tsfrontend/common/types/responses.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/ClickHouseConfigForm.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/ConfigForm.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseConnectionCard.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetup.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/__tests__/clickhouseConfig.test.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseConfig.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/index.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/warehouseFormUtils.ts
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2e4d7c66-6d0d-4cdb-9993-bd0df48ed25f
📒 Files selected for processing (2)
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/ClickHouseConfigForm.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/warehouseFormUtils.ts
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aa770770-55e9-4904-84af-0dc8a3279710
📒 Files selected for processing (1)
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/ClickHouseConfigForm.tsx
| if (result.status === 'connected') { | ||
| setTestState('connected') | ||
| setTestDetail(null) | ||
| toast('Connection verified') | ||
| } else { | ||
| setTestState('errored') | ||
| setTestDetail(result.status_detail) | ||
| toast( | ||
| result.status_detail || 'Connection failed — check your credentials', | ||
| 'danger', | ||
| ) | ||
| } | ||
| } catch { | ||
| setTestState('errored') | ||
| setTestDetail(null) | ||
| toast('Failed to test connection', 'danger') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Discard test results for superseded form values.
A request can resolve after setField resets the test state for changed connection fields. Its stale success then sets connected, enabling save for an untested configuration. Track a monotonically increasing test revision (incremented on field changes) and ignore success/error results whose revision is no longer current.
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Frontend for the
clickhousewarehouse connection type, gated behind theclickhouse_warehouseflag (off by default). Builds on the backend in #8020 and the stateless test endpoint in #8060.status_detailfield: the status tooltip shows the failure reason when the connection is errored, and the test-connection toast reports it instead of a generic message.credentials, read-onlystatus_detail, newtestWarehouseConnectionConfigmutation with aWarehouseConnectionTestResultresponse type.How did you test this code?
Unit tests for the form validation, payload and test-flow helpers (
npm run test:unit -- --testPathPatterns=clickhouseConfig),npm run lint,npm run typecheck(no new errors against the baseline).Manually: enable the
clickhouse_warehouseflag, go to Environment Settings → Warehouse, test and connect a ClickHouse instance (both reachable and unreachable), save after a failed test to check the warning, then edit it and re-test with changed connection details.